Mag l1a compression #801
Mag l1a compression #801
Conversation
maxinelasp
requested review from
vmartinez-cu and
laspsandoval
and removed request for
a team
|
pre-commit.ci autofix |
for more information, see https://pre-commit.ci
…essing into mag_l1a_compression
|
Nice job with your doc strings and comments. The details are helpful |
There was a problem hiding this comment.
Epic effort maxine, thank you!
I have left you lots of comments (sorry!). Lots are just style and readability stuff - i know how hard it is to make this compression logic comprehendable so I feel your pain.
Think my only other query is on error handling. I think it should be a bit more defensive and if you get any bad unexpected values it means something awful has happened and the whole packet should be considered a stream of NaNs and an error raised somewhere so we can investigate..
Thank you!
| # the range | ||
| uncompressed_vector_size = compression_width * 3 + 2 | ||
| # plus 8 to get past the compression width and range data section | ||
| first_vector_width = uncompressed_vector_size + 8 |
There was a problem hiding this comment.
I dont understand the +8 here - you are aready skipping 8 bits in bit array on line 546?
There was a problem hiding this comment.
This is the index of the end of the first vector, so it's uncompressed_vector size long, but we need to skip past the first 8 bits - so the end index is uncompressed_size plus 8. I can move the +8 into the slice in line 546, if that makes more sense?
|
@alastairtree @tech3371 @sdhoyt @laspsandoval Thank you so much for your reviews! I have addressed all the comments on this PR, added some additional tests, and had the output validated by the MAG team. Could I get a re-review and maybe an approval from you? |
|
There are a couple of comments of mine that you "thumbs up'd" / replied to but didn't actually change, which is fine but might be worth checking you have pushed all your changes if not. Otherwise looks good! |
|
@alastairtree Thanks for that final review! I will double check that I addressed everything, there were a few comments that I preferred to leave as-is, but I definitely want to make sure I decided on everything. If there are any comments that you definitely want to see changed, please let me know. |
|
@alastairtree Thanks for pointing out those comments, I guess I resolved a bunch of them in some kind of fugue state on Friday afternoon and didn't actually fix the things you were pointing out! I went back in and I'm addressing your comments, including a bunch on the tests that I missed. Sorry about that, I got a little over eager to finish this up 😅 |
maxinelasp
merged commit 1effdb0
into
IMAP-Science-Operations-Center:dev
Change Summary
This is a pretty involved PR. I would like to thank in advance anyone who reviews. I don't think breaking this code up further would make sense, or I would have done so.
I'd recommend starting by reading the docstring for "process_compressed_vectors" in mag_l1a_data.py, as it gives a good overview on the overall algorithm.
Note: "process_uncompressed_vectors" was moved into a new method but otherwise unchanged.
Overview
Updated Files
mag_l1a_data.py
test_mag_l1a
mag_l0_data
Testing
Testing is pretty extensive for this change, since it's so complex. I generated test data primarily using a piece of code that the MAG team provided for me.
Closes issue #727